Conversation
There was a problem hiding this comment.
Can be tested by invoking eg. the snippet below (please modify as needed) somewhere in initTunnelManagerOperation (AppDelegate), after tunnel manager is fully initialized. Account expired system notification cannot be tested this way due to it working in a different manner.
Task {
try await delay(seconds: 5)
print("Simulate close to expiry, 3 days")
self.tunnelManager.simulateAccountExpiration(option: .closeToExpiry(days: 3))
try await delay(seconds: 5)
print("Simulate close to expiry, 2 days")
self.tunnelManager.simulateAccountExpiration(option: .closeToExpiry(days: 2))
try await delay(seconds: 5)
print("Simulate close to expiry, 1 day")
self.tunnelManager.simulateAccountExpiration(option: .closeToExpiry(days: 1))
try await delay(seconds: 5)
print("Simulate close to expiry, 0 days")
self.tunnelManager.simulateAccountExpiration(option: .closeToExpiry(days: 0))
try await delay(seconds: 5)
print("Simulate expired account")
self.tunnelManager.simulateAccountExpiration(option: .expired)
try await delay(seconds: 5)
print("Simulate active account")
self.tunnelManager.simulateAccountExpiration(option: .active)
}
Reviewable status: 0 of 8 files reviewed, all discussions resolved
5ddd0e9 to
ce3712a
Compare
buggmagnet
left a comment
There was a problem hiding this comment.
It looks like you forgot to provide the implementation for delay, Task.sleep did the trick however.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/Notifications/Notification Providers/AccountExpiry.swift line 85 at r1 (raw file):
// Used to compare dates with a precision of a minimum of seconds. var secondsPrecision: Date { Date(timeIntervalSince1970: TimeInterval(Int(timeIntervalSince1970)))
I have to say, it feels really convoluted to do this. I think we could have done entirely the same thing just using a single point in time, and calendar operations.
But it seems to work pretty well, so I'll leave it at that.
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 100 at r1 (raw file):
let dateComponents = Calendar.current.dateComponents( [.second, .minute, .hour, .day, .month, .year], from: Date().addingTimeInterval(1) // Giving some leeway.
Can we clarify what "giving some leeway" means here ?
rablador
left a comment
There was a problem hiding this comment.
Ah, right!
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPN/Notifications/Notification Providers/AccountExpiry.swift line 85 at r1 (raw file):
Previously, buggmagnet wrote…
I have to say, it feels really convoluted to do this. I think we could have done entirely the same thing just using a single point in time, and calendar operations.
But it seems to work pretty well, so I'll leave it at that.
Indeed, done.
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 100 at r1 (raw file):
Previously, buggmagnet wrote…
Can we clarify what "giving some leeway" means here ?
Yep, done.
ce3712a to
57ed772
Compare
buggmagnet
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion
mojganii
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 117 at r2 (raw file):
} let accountHasExpired = deviceState.accountData?.isExpired == true
nit : we can remove the equity expression. an use the variable for the next line
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 120 at r2 (raw file):
let accountHasRecentlyExpired = deviceState.accountData?.isExpired != previousDeviceState.accountData?.isExpired self.accountHasRecentlyExpired = blockedStateByExpiredAccount && accountHasExpired && accountHasRecentlyExpired
isn't enough to compare the previous one state with the new one to realise if the account has been expired recently or not? must we evaluate the tunnel state for blocking state with the reason to be sure it's blocked? I feel we are over-evaluating to figure out if it's expired recently or not.
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 146 at r2 (raw file):
} private var formattedRemainingDurationBody: String? {
nit : I think we can make it better :
Code snippet:
private var formattedRemainingDurationBody: String? {
guard !accountHasRecentlyExpired, let daysRemaining = accountExpiry.daysRemaining(for: .system)?.day else {
return expiredText
}
return daysRemaining == 1 ? singleDayText : multipleDaysText
}
rablador
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 120 at r2 (raw file):
Previously, mojganii wrote…
isn't enough to compare the previous one state with the new one to realise if the account has been expired recently or not? must we evaluate the tunnel state for blocking state with the reason to be sure it's blocked? I feel we are over-evaluating to figure out if it's expired recently or not.
I'm adding extra security here to make sure we don't accidentally spam users with notifications. Basically making sure that:
- Blocked state is because of account expiry, so that we don't trigger for any kind of blocked state.
- Current state is blocked state.
- Previous expiry state is not the same as current expiry state. This last check alone can't decide if we went from expired -> not expired, or the other way around. That's why I combine 2) and 3).
9d288e4 to
fbb3e81
Compare
rablador
left a comment
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 117 at r2 (raw file):
Previously, mojganii wrote…
nit : we can remove the equity expression. an use the variable for the next line
Done.
ios/MullvadVPN/Notifications/Notification Providers/AccountExpirySystemNotificationProvider.swift line 146 at r2 (raw file):
Previously, mojganii wrote…
nit : I think we can make it better :
This doesn't work since no .day should return nil rather than expiredText. This will in turn make notificationRequest return nil, which will prevent a notification being sent. accountHasRecentlyExpired will check that account expiry was recent, but for all other cases (account expired sometime in the past and no longer relevant) there should be no notification date to trigger.
fbb3e81 to
d73de8a
Compare
rablador
left a comment
There was a problem hiding this comment.
Did some updates to fix the last system notification on account expiry. Code got less convoluted as well.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)
buggmagnet
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)
mojganii
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion
d73de8a to
2e2c318
Compare
|
🚨 End to end tests failed. Please check the failed workflow run. |
Currently the iOS app sends but a single notification about running out of time.
We should schedule 4 in-app notifications, with improved copy:
We should also schedule 3 system notifications, with improved copy:
This change is